Skip to content

Optimize the unquoted arguments characterwise walker #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

danielshahaf
Copy link
Member

See the log messages for details. tl;dr, running the main highlighter on its own 0.7.1 tag:

  • 23% faster with interactivecomments on
  • 50% faster with interactivecomments off

And if on top of that the same optimization is applied to the double-quoted highlighter, performance drops by 3% (relative to the 23% faster state).

Before this patch:

num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1)    3       33410.81 11136.94   98.51%  19277.07  6425.69   56.84%  _zsh_highlight_main_highlighter_highlight_list
19)    1       33916.21 33916.21  100.00%      5.27     5.27    0.02%  _zsh_highlight

With this patch:

num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1)    3       27167.49  9055.83   98.17%  18754.77  6251.59   67.77%  _zsh_highlight_main_highlighter_highlight_list
19)    1       27674.40 27674.40  100.00%      5.39     5.39    0.02%  _zsh_highlight

----

And if test-zprof.zsh is changed to not set interactivecomments:

num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1) 13360       36029.12     2.70   83.56%  30304.23     2.27   70.28%  _zsh_highlight_main_highlighter_highlight_argument
21)    1       43117.76 43117.76  100.00%      4.52     4.52    0.01%  _zsh_highlight

num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1) 13360       14782.89     1.11   68.12%   9163.42     0.69   42.23%  _zsh_highlight_main_highlighter_highlight_argument
21)    1       21699.93 21699.93  100.00%      4.17     4.17    0.02%  _zsh_highlight
num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1)    3       25689.17  8563.06   98.15%  18422.01  6140.67   70.38%  _zsh_highlight_main_highlighter_highlight_list
 2) 32390        5706.13     0.18   21.80%   2315.68     0.07    8.85%  _zsh_highlight_main_highlighter_highlight_argument
19)    1       26173.33 26173.33  100.00%      5.27     5.27    0.02%  _zsh_highlight

Interestingly, if I make the change in this diff to
_zsh_highlight_main_highlighter_highlight_double_quote —

>     diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
>     index da6ab2b..bb17618 100644
>     --- a/highlighters/main/main-highlighter.zsh
>     +++ b/highlighters/main/main-highlighter.zsh
>     @@ -1462,10 +1462,13 @@ _zsh_highlight_main_highlighter_highlight_double_quote()
>        local i j k ret style
>        reply=()
>
>     -  for (( i = $1 + 1 ; i <= $#arg ; i += 1 )) ; do
>     +  (( i = $1 ))
>     +  while (( ++i <= $#arg )); do
>     +    i=${arg[(ib.i.)[\"\`\$\\\\${histchars[1]}]]}
>          (( j = i + start_pos - 1 ))
>          (( k = j + 1 ))
>          case "$arg[$i]" in
>     +      ("")  break;;
>            ('"') break;;
>            ('`') saved_reply=($reply)
>                  _zsh_highlight_main_highlighter_highlight_backtick $i

— it actually makes things measurably slower (!), even on input that has
a large number of pasted double-quoted strings: on «BUFFER=": ${(r.8*1500..foo"bar".):-}"»
the slowdown is (1123.24ms / 1091.06ms = 1.0295).  Therefore, I won't be
committing that change.
@phy1729
Copy link
Member

phy1729 commented May 24, 2020

Sorry for the late review. Looks good. I wonder if storing $#arg would have any affect on the speed.

@danielshahaf
Copy link
Member Author

Thanks. As storing anything involves a malloc() (for the Param's value) and unquoted words encountered in command lines are usually short, I suspect it'll usually be faster to just do $#arg (which does a ztrlen(), IIRC).

Something like eval $(which vcs_info)<TAB> would be a counterexample.

I'll measure.

@danielshahaf
Copy link
Member Author

master:

19)    1       26238.39 26238.39  100.00%      5.35     5.35    0.02%  _zsh_highlight
19)    1       26429.15 26429.15  100.00%      5.52     5.52    0.02%  _zsh_highlight
19)    1       26223.44 26223.44  100.00%      5.27     5.27    0.02%  _zsh_highlight

master with the following patch:

patch to memoize `$#arg`
diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index 9bf0e3a..1a86daf 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -1282,6 +1282,7 @@ _zsh_highlight_main_highlighter_highlight_argument()
 {
   local base_style=default i=$1 option_eligible=${2:-1} path_eligible=1 ret start style
   local -a highlights
+  readonly -i arg_len=$#arg
 
   local -a match mbegin mend
   local MATCH; integer MBEGIN MEND
@@ -1321,7 +1322,7 @@ _zsh_highlight_main_highlighter_highlight_argument()
 
   # This loop is a hot path.  Keep it fast!
   (( --i ))
-  while (( ++i <= $#arg )); do
+  while (( ++i <= arg_len )); do
     i=${arg[(ib.i.)[\\\'\"\`\$\<\>\*\?]]}
     case "$arg[$i]" in
       "") break;;
19)    1       27659.11 27659.11  100.00%      5.27     5.27    0.02%  _zsh_highlight
19)    1       26532.46 26532.46  100.00%      5.31     5.31    0.02%  _zsh_highlight
19)    1       27005.08 27005.08  100.00%      5.28     5.28    0.02%  _zsh_highlight

Conclusion: not to memoize, apparently, unless the test case is unrepresentative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants